fix: example deprecation warnings and improvements #4579
Conversation
Gobot1234
requested review from
hpohekar,
mayankansys,
mkundu1,
prmukherj and
seanpearsonuk
as code owners
github-actions
bot
added
documentation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes deprecation warnings by updating all references from direct solver session settings access (solver.setup.models) to the correct settings-based access pattern (solver.settings.setup.models). Additionally, it adds PEP 723 style dependencies to example scripts and includes minor improvements throughout.
- Updates deprecated direct access patterns to settings-based access across all test files and examples
- Adds PEP 723 dependency declarations to example scripts for standalone execution
- Includes miscellaneous code improvements and cleanups
Reviewed Changes
Copilot reviewed 67 out of 68 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| tests/ | Updates test files to use solver.settings instead of direct access patterns |
| examples/ | Adds PEP 723 dependency headers and updates deprecated settings access |
| src/ | Updates core library files to use settings-based access pattern |
| doc/ | Updates documentation examples to reflect new access patterns |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Anyone have any opinions on #4512 (review)? |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 67 out of 68 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 67 out of 68 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ------------------------- | ||
|
|
||
| Following the introduction of the new format, the traditional method remains available for those | ||
| Following the introduction of the new format, the traditional method remains available for those |
There was a problem hiding this comment.
This sentence is out of place and lacks context:
“Following the introduction of the new format, the traditional method remains available for those who prefer the existing hierarchy.”
It doesn’t belong at the start of this section and should instead be integrated into the previous section—after introducing the new-style API—so that the relationship between the new and existing APIs is clear and non-redundant. The goal is simply to mention that the existing hierarchy-based API remains fully supported and is not deprecated.
Below are suggested rewrites for both sections.
Suggested rewrite for the previous section (new-style API introduction)
To simplify access to solver settings and improve readability, you can now instantiate settings objects directly using a concise constructor-style syntax. This avoids navigating the full hierarchy of solver settings while preserving the same functional capabilities.
<Example>
The traditional hierarchy-based API remains fully supported. The new syntax is an additional, more convenient option and not a replacement.
Suggested rewrite for the Accessing solver settings section
The hierarchy-based settings API continues to be available for users who prefer explicit navigation of the settings structure. For example, when working with string lists that support wildcard expansion:
<Code>
Co-authored-by: Sean Pearson <93727996+seanpearsonuk@users.noreply.github.com>
|
@Gobot1234 I made a number of remarks unconnected with the changes but I didn't want to miss the opportunity to fix some badly broken windows. My comments can be addressed either here or subsequently. For any that we do subsequently, we need to generate Issues from the comments. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 67 out of 68 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 67 out of 68 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| assert s.split("\n")[-2].split("(")[0] == r"<solver_session>.file.read_case" | ||
| else: | ||
| assert ( | ||
| s.split("\n")[-2].split("(")[0] | ||
| == r"<solver_session>.settings.file.read_case" | ||
| ) |
There was a problem hiding this comment.
The version comparison logic is inverted. For Fluent version >= v251, the assertion expects <solver_session>.file.read_case, but based on the PR description (updating to use solver.settings.*), newer versions should use the .settings. prefix. The conditional branches should be swapped.
| assert s.split("\n")[-2].split("(")[0] == r"<solver_session>.file.read_case" | |
| else: | |
| assert ( | |
| s.split("\n")[-2].split("(")[0] | |
| == r"<solver_session>.settings.file.read_case" | |
| ) | |
| assert ( | |
| s.split("\n")[-2].split("(")[0] | |
| == r"<solver_session>.settings.file.read_case" | |
| ) | |
| else: | |
| assert s.split("\n")[-2].split("(")[0] == r"<solver_session>.file.read_case" |
| solver.tui.file.read_case(case_path) | ||
| timeout_loop( | ||
| lambda: "<solver_session>.settings.file.read_case" in capsys.readouterr().out, | ||
| lambda: "<solver_session>.file.read_case" in capsys.readouterr().out, |
There was a problem hiding this comment.
The assertion checks for the deprecated access pattern <solver_session>.file.read_case instead of the updated pattern <solver_session>.settings.file.read_case. This contradicts the PR's goal of updating to the new API.
| lambda: "<solver_session>.file.read_case" in capsys.readouterr().out, | |
| lambda: "<solver_session>.settings.file.read_case" in capsys.readouterr().out, |
tests/test_reduction.py
Outdated
| absolute_pressure_expression.definition = "AbsolutePressure" | ||
|
|
||
| s1_min = solver1.fields.reduction.minimum( | ||
| s1_min = solver1.settings.fields.reduction.minimum( |
There was a problem hiding this comment.
The path solver1.settings.fields.reduction is incorrect. Based on other usage in the codebase, it should be solver1.fields.reduction without the .settings prefix.
| s1_min = solver1.settings.fields.reduction.minimum( | |
| s1_min = solver1.fields.reduction.minimum( |
| # --------------------------------------------------------------- | ||
|
|
||
| session = pyfluent.launch_fluent(precision="double", processor_count=2, version="3d") | ||
| session = pyfluent.launch_fluent(precision="double", processor_count=2, dimension=3) |
There was a problem hiding this comment.
Changed parameter from version=\"3d\" to dimension=3. Verify that dimension=3 is the correct parameter name and value for specifying 3D mode, as this represents an API change.
|
@prmukherj / @seanpearsonuk are you okay with the tests that I've had to bump the markers of here? |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 67 out of 68 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
|
|
||
| @pytest.mark.fluent_version(">=24.1") | ||
| @pytest.mark.fluent_version(">=25.1") |
There was a problem hiding this comment.
The minimum Fluent version requirement was changed from >=24.1 to >=25.1, which may unnecessarily restrict test execution on Fluent 24.x versions where the functionality should still work.
| @pytest.mark.fluent_version(">=25.1") | |
| @pytest.mark.fluent_version(">=24.1") |
|
|
||
|
|
||
| @pytest.mark.fluent_version(">=24.1") | ||
| @pytest.mark.fluent_version(">=25.2") |
There was a problem hiding this comment.
The minimum Fluent version requirement was changed from >=24.1 to >=25.2. Unless there's a specific feature dependency on 25.2, this unnecessarily restricts test coverage on earlier compatible versions.
| @pytest.mark.fluent_version(">=25.2") | |
| @pytest.mark.fluent_version(">=24.2") |
|
|
||
|
|
||
| @pytest.mark.fluent_version(">=25.1") | ||
| @pytest.mark.fluent_version(">=25.2") |
There was a problem hiding this comment.
The minimum Fluent version requirement was changed from >=25.1 to >=25.2. This should only be updated if there's a concrete dependency on 25.2 features; otherwise, it reduces test coverage.
| @pytest.mark.fluent_version(">=25.2") | |
| @pytest.mark.fluent_version(">=25.1") |
| import csv | ||
| import math | ||
| import os |
There was a problem hiding this comment.
The csv import is now placed after the header comment but before other standard library imports (math, os, pathlib). Standard library imports should be grouped together following PEP 8 import ordering guidelines.
| import csv | |
| import math | |
| import os | |
| import math | |
| import os | |
| import csv |
Context
When solver session settings access was deprecated directly on solver the code wasn't updated I've also sprinkled in a couple of small improvements to the examples
Change Summary
Updated all references direct access patterns (
solver.setup.models) to the correct settings-based access pattern (solver.settings.setup.models) along with adding PEP 723 styles dependencies to the examples to make them easier to run as standalone scripts.Rationale
Fixes all the deprecation warnings
Impact
Changes basically everywhere